Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve docker-compose by adding healthcheck condition at startup #420

Merged
merged 2 commits into from
Nov 3, 2020

Conversation

jbruggem
Copy link
Collaborator

@jbruggem jbruggem commented Nov 2, 2020

This adds a health check and a dummy service to make sure that we wait for Kafka to be healthy when we start the nodes.

This also adds an environment variable to toggle the capture of logs in tests, which is convenient when debugging complicated test cases. It also adds a formatter to show module, function and PID.

There's some changes due to a mix format, but other than that this MR does not change any elixir code.

@jbruggem jbruggem force-pushed the improve_docker_compose branch 2 times, most recently from 1f9f1fb to 518925e Compare November 2, 2020 08:38
@dantswain
Copy link
Collaborator

Seems like it's not working :( I like the idea, though.

@jbruggem
Copy link
Collaborator Author

jbruggem commented Nov 2, 2020

Seems like it's not working :( I like the idea, though.

The two failures seem to be:

  • a coverall result upload error
  • tests failing in one of the pipelines, but I didn't change any elixir code ? (mix format did, but I trust that :P)

Could it just be pipeline unreliability ?

I can't seem to re-trigger the failed jobs. Would it be a good idea to do that ?

@dantswain
Copy link
Collaborator

Triggered again.

I have seen the coveralls error before and don't have a good solution. I sort of wonder if it's worth keeping coveralls at this point... i like having coverage metrics but it's not like we ever do anything about it. We could keep the coveralls lib installed because it does produce a nice report, but disable the travis integration. That would be for a different PR anyways.

You could also try doing the mix format changes on a separate pr just to be safe? Let's see what the build does.

@jbruggem
Copy link
Collaborator Author

jbruggem commented Nov 3, 2020

I have seen the coveralls error before and don't have a good solution. I sort of wonder if it's worth keeping coveralls at this point... i like having coverage metrics but it's not like we ever do anything about it. We could keep the coveralls lib installed because it does produce a nice report, but disable the travis integration. That would be for a different PR anyways.

It failed only on that this time. I'll disable the travis integration here if you don't mind ?

@jbruggem jbruggem force-pushed the improve_docker_compose branch 2 times, most recently from a3bf468 to a693d2d Compare November 3, 2020 10:48
@jbruggem
Copy link
Collaborator Author

jbruggem commented Nov 3, 2020

@dantswain OK for you if I merge with the coveralls deactivated ?

@dantswain
Copy link
Collaborator

Can you maybe re-enable it and then open a separate PR? I think it's probably the right move but I think it deserves its own review and weigh in from others.

@jbruggem
Copy link
Collaborator Author

jbruggem commented Nov 3, 2020

Can you maybe re-enable it and then open a separate PR? I think it's probably the right move but I think it deserves its own review and weigh in from others.

OK. I don't think we'll manage to get a green pipeline though :)

@dantswain
Copy link
Collaborator

OK. I don't think we'll manage to get a green pipeline though :)

That's fine, we know why.

@jbruggem
Copy link
Collaborator Author

jbruggem commented Nov 3, 2020

Given the low impact, I'll merge this then, since it seems it could help review #414

@jbruggem jbruggem merged commit b687794 into master Nov 3, 2020
@dantswain dantswain deleted the improve_docker_compose branch November 3, 2020 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants